Add Copyright class and copyright_diff() #82 #83 by johnmhoran · Pull Request #85 · aboutcode-org/deltacode

johnmhoran · 2018-03-02T02:40:20Z

Addresses issues #82 and #83, both of which are partial implementations of #43.

* Create 'Copyright' class. * Add 'Copyright' to 'File' object. * Add empty lists for 'copyrights' and 'licenses' fields to 'File.to_dict()'. * Add new tests, fix failing tests. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

* Add copyright_diff(). * Refactor license_diff(). * Add new tests and related test old/new scan pairs. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

steven-esser · 2018-03-05T12:45:36Z

-                    i.score += 15
+                if delta.new_file.licenses == [] and len(delta.old_file.licenses) > 0:
+                    delta.factors.append('license info removed')
+                    delta.score += 15


We should make a method that down

Turn this logic into a function where you pass a score and a string. Then we can replace lines like 207 and 208 with a single function call instead of two line tweaks

Add tests for this as well

steven-esser · 2018-03-05T12:53:05Z

+                if delta.new_file.copyrights == [] and len(delta.old_file.copyrights) > 0:
+                    delta.factors.append('copyright info removed')
+                    delta.score += 10
+                    return


Probably best that we score these the same. This will also allow for you to combine the logic into a single if statement

steven-esser · 2018-03-05T12:55:08Z

+
+                if ((new_statements != old_statements) or
+                        (new_holders != old_holders) or
+                        (new_authors != old_authors)):


I think we should ignore looking at copyright authors for now unless we have a good reason to use them.

Also I believe we need to rely more on holders. This needs some thought though

@MaJuRG I've made this change -- let me know when you'd like to discuss how we could place greater reliance on holders.

steven-esser · 2018-03-05T12:56:36Z

+        self.statements = dictionary.get('statements')
+        self.holders = dictionary.get('holders')
+        self.authors = dictionary.get('authors')
+


Like I mentioned above, lets remove authors.

steven-esser · 2018-03-05T12:58:38Z

+    'file' dictionary.
+    """
+    def __init__(self, dictionary={}):
+        self.statements = dictionary.get('statements')


If you haven’t already, you should add tests cases where a files have large numbers of copyrights holders and statements to see if we choke somewhere, espically in the output

Also tests where there are strange characters or accent marks in the copyright statements/holders.

All of these additional test cases I mentioned should probably come from scancode generated output directly as opposed to crafting the test object by hand.

@MaJuRG Since we want to use ScanCode-generated output, do you have any codebases in mind that satisfy the characteristics you describe?

I've started to work my way through the codebases we've worked with (openssl, zlib et al.) but I've not yet seen large numbers of copyright holders/statements or unusual characters.

@johnmhoran You will probably just have to hand-create a file or files that contain a bunch of copyright statements

OK. Thanks, @MaJuRG .

@MaJuRG I'm finding that an error is thrown when I include a French accent character in the copyright statements or holders value, e.g., "é". The error is thrown even if I comment out the character (presumably because even comments are parsed). Unicode and UTF-8 do not throw an error.

... "statements": [ "U+00E9", "\xc3\xa9" # "é" ...

SyntaxError: Non-ASCII character '\xc3' in file C:\code\nexb\dev\deltacode\tests\test_models.py on line 1073, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

While PEP 263 gives some suggestions, it's not clear to me how we can apply these to handle our input. I've done some searching in the ScanCode repo -- surely ScanCode must be able to handle such characters -- but have not yet found how ScanCode addresses this.

Looks like this issue might be addressed in scancode-toolkit/src/commoncode/text.py?

@johnmhoran where is this "statements" located?

@MaJuRG Per your suggestion above, I'm hand-crafting old and new files in a new test, test_Copyright_unusual_characters(), in test_models.py.

@MaJuRG Would it be easier for you if I commit and push? Except for this one failing test, it's ready for your review.

* Add scoring method to Delta -- Delta.add_score(). * Call new scoring method from DeltaCode.license_diff() and DeltaCode.copyright_diff(). * Add tests for Delta.add_score(). * Modify score for 'copyright info added'. * Remove references to copyright authors. * Add tests cases where a files have large numbers of copyright holders and statements. * Add tests cases where a files have unusual characters -- but errors thrown with French accent characters. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

steven-esser · 2018-03-06T20:38:13Z

                if len(delta.new_file.licenses) > 0 and delta.old_file.licenses == []:
-                    delta.factors.append('license info added')
-                    delta.score += 20
+                    delta.add_score(20, 'license info added')


This needs to be renamed to something like delta.update.
We are not only adding to the score, we are also adding to factors etc and possibly more in the future.

steven-esser · 2018-03-06T20:40:51Z

+    def add_score(self, score=0, factor=''):
+        """
+        For each Delta object identified in DeltaCode.license_diff() or
+        DeltaCode.copyright_diff(), add the score to the object's 'score'


We do not need ot mention these funtions here. Simply tell me what this update function does.

steven-esser · 2018-03-06T20:40:53Z

        self.factors = []
        self.score = score

+    def add_score(self, score=0, factor=''):


like stated above, this needs to be renamed to update.

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

johnmhoran · 2018-03-06T21:14:14Z

@MaJuRG Done except for the question of how to test unusual characters like the French é in copyright statements/holders. How shall we handle that testing?

steven-esser · 2018-03-06T21:23:28Z

@johnmhoran I do not know the best way.

Have you:

created some file with copyright statement that has that accent?
scan that file with scancode using -clip option
placed that result scanfile in the data direcoty to be used as a test case?
run the test?

I ask because I dont really have a context to the error above and therefore cannot point you in the right direction.

steven-esser · 2018-03-06T21:28:30Z

For example, scancode results may handle things like this implicitly, and we do not need to worry about it. OR it could be that we need to sanitize these strings at some point along the line depending on what operations we are performing.

Unfortunately, this is a side effect of python2 specific problems w.r.t strings and character encodings.

It may be that we push that particular issue off in favor of python3 migration, but we need more details. We may not care at all.

More research on your part is always something you can do as well; I will not be able to really provide specific code examples etc until late weds or thurs

johnmhoran · 2018-03-06T21:43:05Z

@MaJuRG Understood. I did a fair amount of research but couldn't figure out how to apply it to the DeltaCode/ScanCode context. However, thinking about the process, I expect that my prior approach -- hand-crafting the input inside the test itself -- meant that any existing ScanCode/commoncode remediation was not encountered by the input.

I like your suggestion -- will create separate files, scan with ScanCode and use for testing.

* Added scans and tests for unusual characters like French and German letters with accent marks. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

johnmhoran · 2018-03-08T19:56:30Z

@MaJuRG Working on issue #71 branched from my branch for issue #83, I noticed two print statements in the test test_Delta_to_dict_Copyright_unusual_characters() and have removed them.

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

johnmhoran added 3 commits February 26, 2018 15:32

Add 'Copyright' class #82

00b6e7a

* Create 'Copyright' class. * Add 'Copyright' to 'File' object. * Add empty lists for 'copyrights' and 'licenses' fields to 'File.to_dict()'. * Add new tests, fix failing tests. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Add copyright_diff() to DeltaCode object #83

f8f5861

* Add copyright_diff(). * Refactor license_diff(). * Add new tests and related test old/new scan pairs. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Add additional copyright-related tests #83

f1c9f4d

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

johnmhoran assigned steven-esser Mar 2, 2018

steven-esser suggested changes Mar 5, 2018

View reviewed changes

steven-esser suggested changes Mar 6, 2018

View reviewed changes

Rename method to 'delta.update()' #82 #83

d2cb79d

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Test copyrights for unusual characters #82 #83

195eb00

* Added scans and tests for unusual characters like French and German letters with accent marks. Signed-off-by: John M. Horan <johnmhoran@gmail.com>

Remove print statements #82 #83

4199bcd

Signed-off-by: John M. Horan <johnmhoran@gmail.com>

steven-esser approved these changes Mar 8, 2018

View reviewed changes

steven-esser merged commit 5459956 into develop Mar 8, 2018

This was referenced Mar 8, 2018

Add Copyright class #82

Closed

Add copyright_diff() to DeltaCode object #83

Closed

Uh oh!

Uh oh!

Conversation

johnmhoran commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnmhoran commented Mar 6, 2018

Uh oh!

steven-esser commented Mar 6, 2018

Uh oh!

steven-esser commented Mar 6, 2018

Uh oh!

johnmhoran commented Mar 6, 2018

Uh oh!

johnmhoran commented Mar 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants